-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] feat: per module inlineDynamicImport #3299
base: master
Are you sure you want to change the base?
[WIP] feat: per module inlineDynamicImport #3299
Conversation
it will merge conflict with #3295 |
9a334b0
to
ce73ae8
Compare
Codecov Report
@@ Coverage Diff @@
## master #3299 +/- ##
==========================================
+ Coverage 93.12% 93.14% +0.01%
==========================================
Files 170 170
Lines 5967 5982 +15
Branches 1780 1785 +5
==========================================
+ Hits 5557 5572 +15
Misses 219 219
Partials 191 191
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I added some tests in order to get a feeling for what this feature does. Unfortunately it turns out that this does completely prevent conditional execution of imports (besides the fact that it messes up execution order by executing code synchronously that should rather be executed async). I have no idea how this could be solved, i.e. what code Rollup could sensibly generate to address this. If you think the feature is still useful as it is, then we can definitely discuss this.
@@ -19,6 +19,11 @@ export function assignChunkColouringHashes( | |||
Uint8ArrayXor(module.entryPointsHash, currentEntryHash); | |||
} | |||
|
|||
for (const { resolution } of module.dynamicImports) { | |||
if (resolution instanceof Module && resolution.inlineDynamicImport) { | |||
module.dependencies.push(resolution); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think this is how we should do it. The reason is that module.dependencies
is responsible for which imports the chunk has that this module ends up in. This means that if the target of the inlined dynamic import ends up in a different chunk than the import expression, this chunk will generate both the dynamic import as well as a static one. I added a test that show-cases this.
@@ -0,0 +1,5 @@ | |||
import './generated-inlined.js'; | |||
|
|||
import('./generated-inlined.js').then(console.log); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice how here, we have both a static as well as a dynamic import to the chunk containing the shared dynamic import target. The reason is because the dynamic import is added to the modules dependencies.
var inlined = /*#__PURE__*/Object.freeze({ | ||
__proto__: null, | ||
value: value | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the single biggest problem I encountered with the current implementation of this feature: The dynamic imports are no longer conditionally executed, they are immediately and synchronously executed. See this example: If globalThis.unknown
is false, the namespace is not read, but the side-effect log statement of the dynamic import is still triggered. I am not sure this feature is valuable if this does not work. However I have no idea how to actually make it work. If we start wrapping code in conditionally executed function closures, then there would still be the problem if a dynamically imported module that is inlined has itself a static dependency that ends up in yet another chunk—how should this be handled? There are no conditional static imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, it could technically be done by first dynamically importing the static dependencies that are in other chunks and THEN executing the conditionally imported code. But it would become quite a bit bigger than it is now because then we need to think about the whole chunk rendering process, not only the colouring algorithm. If we want to go there, I would suggest we should collect a few scenarios with expected sample code first before thinking about the implementation.
eca6a6c
to
71d20c9
Compare
cbd94e6
to
f1538ce
Compare
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
This PR allows to specify if a dynamically imported module should be inlined or not.
This allows rollup to:
The WASM use case is interesting, since in order to load WASM eficiently, async is a must, a dynamic import covers the use case perfectly:
The problem is that currently this import() will create two files (and meaning one extra round-trip):
file.wasm.js
chunk (the js code that loads the WASM):and the
file.wasm
asset.This new features allows to inline the JS, since the WASM is already loaded from a different asset